update the dependency and temporal filter for python_cmr 0.10.0 #520
Conversation
|
Hmm... |
| date_from: Optional[Union[str, dt.date]] = None, | ||
| date_to: Optional[Union[str, dt.date]] = None, |
There was a problem hiding this comment.
| date_from: Optional[Union[str, dt.date]] = None, | |
| date_to: Optional[Union[str, dt.date]] = None, | |
| date_from: Optional[Union[str, dt.date, dt.datetime]] = None, | |
| date_to: Optional[Union[str, dt.date, dt.datetime]] = None, |
I know it's not necessary or even correct since datetime is a subclass of date, but I think from a documentation standpoint this will be clearer to users.
There was a problem hiding this comment.
Currently the type hint does not effect the documentation, because the docstring includes a "type". Do you want that part removed from the docstring?
If type hints are being included to help developers, then I would want them to be technically correct. If type hints are being included to help users ... well, I fell like that's a little lazy actually. Users don't benefit a whole lot from "dt.date" without knowing what "dt" is. And do they know what "Optional[Union[...]]" means? Relying on auto documentation is nice ... if it's good documentation.
You are welcome to modify however you think its best.
There was a problem hiding this comment.
Personally, when type annotations are provided, I drop type information from the docstrings. Otherwise, it's all too easy for the docstrings to become out of sync with the type annotations, leading to confusion, misinformation, and frustration. Modern doc generators, including mkdocs, will make use of the type annotations to put such information in the docs. Doc generators should also use canonical names for things as well, meaning that even though the code may refer to the datetime module as dt, the docs will show the name datetime, so there's no need for the reader to know about any aliasing within the code.
| date_from: Optional[Union[str, dt.date]] = None, | ||
| date_to: Optional[Union[str, dt.date]] = None, |
There was a problem hiding this comment.
| date_from: Optional[Union[str, dt.date]] = None, | |
| date_to: Optional[Union[str, dt.date]] = None, | |
| date_from: Optional[Union[str, dt.date, dt.datetime]] = None, | |
| date_to: Optional[Union[str, dt.date, dt.datetime]] = None, |
|
@itcarroll, in addition to addressing @jhkennedy's comments, you'll need to resolve conflicts coming from the main branch. |
|
@itcarroll, let's schedule a pairing session so I can help you sort out the build failures you're encountering. Regarding the vcrpy failures, it might behoove us to wait for @doug-newman-nasa's PR (#528) to land beforehand, so you don't accidentally commit an access token like I did. |
|
@itcarroll, I just merged @doug-newman-nasa's PR for #528, so I'm going to pick up this PR now to iron out the kinks. |
|
@itcarroll, I see that you added |
|
Darn, I meant to leave a comment on that and now I can't find the relevant issue. Roughly .... When I updated poetry.lock for python-cmr 0.10, an update to lxml caused document generation to fail. The nbconvert we are pinned to by mkdocs-jupyter seemed unlikely to be updated to express a new version constraint on lxml. Since it's only a dev dependency, I hoped a direct import would be an acceptable solution. |
|
No worries. That's sufficient explanation. Thanks! |
|
@itcarroll, please take a look at #546 That's the PR I just submitted based upon your original PR. I removed the Let me know if everything looks good. If so, then I'll have you close your PR. |
|
Thank you very much @chuckwondo, looks great. Please proceed! |
Version 0.10.0 of the
python_cmrpackage correctly handles a broader range of date-like inputs for the temporal filters, due to our input via nasa/python_cmr#30. This PR increments our version dependency, strips our own preprocessing of date-like inputs, and modifies tests accordingly.Fixes #190
Fixes #330
📚 Documentation preview 📚: https://earthaccess--520.org.readthedocs.build/en/520/